common: Generalized XML-style tool-call parsing with streaming support (GLM 4.5/4.6 + MiniMax M2 + SeedOSS) #16932
Conversation
|
I'm looking forward to get this PR merged! @hksdpc255 Does it require a custom jinja template from the previous PR or it works good as is? |
|
For now, I’d recommend using a custom template if you’re running more complex workloads. Edit: The official template is now working properly. There’s no longer need for a custom template. Edit2: Official template support for Minimax-M2 has been removed. See comment and ochafik/minja#7 (comment) for details. |
|
FYI I've updated (my fork of) Minja w/ support for GLM 4.6's template. |
|
@ochafik Excellent work! Once llama.cpp syncs your changes, some parts of this PR can be safely removed. However, there are still a few small patches needed — for example, replacing |
|
Currently, the official Minimax-M2 chat template fails to run tool calls because |
@hksdpc255 Both should be supported. The confusing error you probably got was because minja implements As for And please feel free to file bugs on https://github.com/ochafik/minja, it's should be cleaner to add syntax support there than to patch things up in llama.cpp. |
|
@ochafik Thank you for pointing that out. I’m currently applying your suggested fix in llama.cpp and will test whether it works as expected. Thanks again for the help! |
|
Good news! The Minimax M2 tool call is now working. I’ll push the fix later. |
|
Screen shot for Zed editor: Model: unsloth's UD-Q3_K_XL |
|
Hi @hksdpc255 , Model: unsloth--MiniMax-M2-GGUF Q8_0 ./llama-cli \
-m /models/hub/models--unsloth--MiniMax-M2-GGUF/snapshots/*/Q8_0/MiniMax-M2-Q8_0-00001-of-00005.gguf \
-ngl 99 \
-sm layer \
-ts 1,1,1,1,1,1,1,1 \
-c 78000 \
-t 16 \
--jinja \
-iOutput: > what is the capital of france?
Okay, the user asked a straightforward question: "What is the capital of France?" This is basic geography knowledge, so the answer should be simple. I don't need to overcomplicate things.
Hmm, maybe the user is just testing if I know basic facts, or perhaps they're new to this kind of question. Either way, the response should be clear and concise. No need for extra details unless they ask follow-ups.
I recall that Paris is the capital of France. It's one of the most well-known capitals globally, so this should be an easy one. The user might be a student working on homework, or someone prepping for trivia. Or maybe they're just curious—either way, I should confirm it confidently.
No signs of confusion or deeper needs here. The question is very direct. I'll just state the answer plainly. If they want more info later, like landmarks or history, they'll ask. For now, keep it simple: Paris is the capital.
Wait, should I add that it's also a major cultural hub? Nah, overcomplicating it. Just the fact. Done.
</think>
The capital of France is **Paris**.
Paris is not only the political center but also a major cultural, economic, and gastronomic hub, famous for landmarks like the Eiffel Tower, the Louvre Museum, Notre-Dame Cathedral, and the Champs-Élysées. |
|
@emuchogu Sorry, I haven’t tested it with If you want I’m not sure whether |
|
I’ve reverted my previous PR (reasoning-format-minimax-m2) and merged PR #16932 into my testing-branch16 for isolated testing. Without this PR :Streaming, no initial <think> tag in the output: Curl without streaming no initial <think> tag in the output : With this PR :Streaming : Curl without streaming, no initial <think> tag in the output : |
|
Oh! It seems you’re using non-streaming mode. I can now reproduce your issue with Let me dig into what’s happening… |
Yes, exactly: it works correctly in streaming mode (tested through the SvelteUI, which specifically designed to be debug-friendly without needing curl -N), but not in non-streaming mode. |
|
Toolcall debug on SvelteUI with your #16932 + #16618 :) Custom JSON : 
|
|
@ServeurpersoCom The problem is that I added some code that makes it fall back to llama.cpp’s original parser when there are no tools, so the new parser is never called. Lines 2748 to 2753 in af5216e Simply deleting the code above should fix the issue. I’ll run more tests before pushing a new commit. 
|
I’ve successfully tested it without these lines of code and confirmed it works as expected for streaming / non streaming / reasoning_content / toolcall |
|
I just realized this, and it seems strange: shouldn’t --reasoning-format none completely bypass any parsing logic instead of still going through it? It’s meant to be the raw passthrough mode for observing the model’s native output. The .cpp files are already becoming huge and monolithic, making them harder to touch or refactor safely. The --reasoning-format options are also poorly named and not very explicit. In the long run, a modular templating system would help avoid piling up even more C++ parsing code. If this work is meant to unify several next-generation parsers, maybe we could add a new keyword to --reasoning-format instead? It’s important to keep none as a truly no-parsing mode, since it’s essential for debugging new models. Also, the current "auto" mode is actually just "deepseek" in practice, so it might be clearer to rename or document it that way to avoid confusion: and your unified detection logic could be implemented directly under auto (or deepseek, since they’re basically aliases) ? |
|
@aaronnewsome It should work out of the box, for both with the official chat template and with Unsloth’s template. |
|
I've checked out and built I start llama server with: In my first quick test, using vscode latest, cline latest, I asked it to create a quick instruction md file for how to deploy a container. Then asked it to add the md to git, commit and push. All seemed to go ok. I really like that Cline does much better at reading the terminal output of the commands. GLM would consistently read the first output, then fail from remaining commands (yes, I've tried all the hacks I could find). Minimax-M2 seemed to do much better. I also appreciate how much faster Minimax-M2 is on the same hardware - now you can see why I'm so keen to get this model running to replace GLM 4.5 Air (the only GLM 4.6 I could get running on my system was the Q2, which performed horribly, got lost in code frequently etc). Cline is also able to use MCP with MiniMax (tested with context7). Most importantly, I was able to use OpenCode with MiniMax-M2. Something that always gave me problems with GLM 4.5-Air (although I still haven't tried any diff edits with OpenCode, which reliably fail with GLM 4.5-Air). 
Thanks for everything you do @hksdpc255 to help bring these tools to all of us who prefer to use local LLM. So far, in my own testing, Minimax-M2 beats ANYTHING that will run on my rig - so if the testing continues to go well, I'll never spin up GLM 4.5-Air again. UPDATE: I was even able to use chrome-devtools mcp AND the take_screenshot tool. it uses a ridiculous amount of memory, consumed the entire context in the chat (even using all of the system DRAM), but Minimax was able to take the screenshot and the analysis of the image data was right on, no errors even though it took forever. I'm impressed. |
|
@hksdpc255 You've put a lot of good work in this PR and I'm starting to get convinced that it should supercede mine, but I'd ask you to do two things: -> remove the template patching code. They way this is done is that you put the proper template in |
|
@aaronnewsome Do you mean the task stops during the tool-call observation loop, or that it fails when handling parallel tool calls? |
|
@pwilkin Thank you for reviewing my code. The template patching logic was removed after your initial review. The only remaining patch now targets the buggy official Minimax-M2 template (see ochafik/minja#7 (comment) ), which ensures that the official template works correctly. So, do you mean that removing this code causes the unmodified official template to stop working? Also, before I move my code into a separate file, I’d like to ask for your opinion: do you think it would be a good idea to make parse_msg_with_xml_tool_calls a member of common_chat_msg_parser? |
I'd wager that around half of the unmodified official templates don't work out of the box with Llama.cpp ;) The expected workflow is as follows: we provide support for the template and release our official supported template (via models/templates/) and then people either bake it into their models during GGUF creation (preferred) or use That's the preferred hotfixing method, not using hardcoded patches in the .cpp code.
For now, just keep it separate, I'll probably do a bigger refactor and then I'll possibly move it. |
|
@pwilkin Done |
pwilkin
left a comment
pwilkin
left a comment
There was a problem hiding this comment.
Just a few cleanup things.
| std::vector<std::string> tool_rules; | ||
| for (const auto & tool : tools) { | ||
| if (!tool.contains("type") || tool.at("type") != "function" || !tool.contains("function")) { | ||
| LOG_INF("Skipping tool without function: %s", tool.dump(2).c_str()); |
| } | ||
| const auto & function = tool.at("function"); | ||
| if (!function.contains("name") || !function.at("name").is_string()) { | ||
| LOG_INF("Skipping invalid function (invalid name): %s", function.dump(2).c_str()); |
| continue; | ||
| } | ||
| if (!function.contains("parameters") || !function.at("parameters").is_object()) { | ||
| LOG_INF("Skipping invalid function (invalid parameters): %s", function.dump(2).c_str()); |
| auto parameters = function.at("parameters"); | ||
| builder.resolve_refs(parameters); | ||
| if (!parameters.contains("properties") || !parameters.at("properties").is_object()) { | ||
| LOG_INF("Skipping invalid function (invalid properties): %s", function.dump(2).c_str()); |
| // tmpl_inputs.now = std::chrono::system_clock::now(); | ||
|
|
||
| minja::chat_template_options tmpl_opts; | ||
| minja::chat_template_options default_tmpl_opts; |
There was a problem hiding this comment.
Think it's safe to delete the "template hacking" parts now.
There was a problem hiding this comment.
We can just leave this in for potential future use.
| // instead of using `chat_template_options.use_bos_token = false`, since these tokens | ||
| // may be needed inside the template / between messages too. | ||
| auto result = tmpl.apply(tmpl_inputs, tmpl_opts); | ||
| auto result = tmpl.apply(tmpl_inputs, tmpl_opts ? *tmpl_opts : default_tmpl_opts); |
| // "<function=calculate_sum>\n" | ||
| // "<parameter=numbers>[1,\n", | ||
| // /* is_partial= */ true, | ||
| // {COMMON_CHAT_FORMAT_SEED_OSS})); |
There was a problem hiding this comment.
Replace this with a proper partial parse test.
There was a problem hiding this comment.
I’m not sure how to write the partial tool call test. Could you help me with that?
|
I checked out the PR (83181f2) to play around with MiniMax M2. It seemed to work great! There is one test I do with every model and that is to convert a mid-level complexity 1300 lines Python script to Rust. Smaller models often do simplified implementation or stubs. MiniMax M2 was doing a pretty fine job there... until llama-server segfaulted. I fired up gdb and collected a backtrace. The callstack is huge, it looks like an endless recursion somewhere deep in the std::regex code. See for yourself: It probably died from stack exhaustion. The crash occurrs around 19080 tokens and is fully reproducible. |
|
@thomasjfox Can you share the log before crashing? The most important line will be something like: |
|
Found an issue: the parser crashes when the LLM generates a start token inside reasoning content. I’ll work on a fix. |
@hksdpc255 Could you please add your template file for GLM 4.5 that works with the PR to models/templates folder in the repo? Thanks |
|
I’ll run full tests on my side including delta.reasoning_content, tool_calls, and agentic loop behavior both on a stock Raspberry Pi 5 setup and on my main server once the PR is ready. |
I think it would be best to wait until you have that fix implemented before I run my experiments again. That way we can determine whether it was the exact issue or something different. I can only crash / upgrade the server in the evenings. 😀 One thing I found out yesterday is that the crash was not related to my exact prompt. I tried a different longish prompt and it also crashed around 19000 - 20000 output tokens. The tests were conducted with the MiniMax M2 q2 and q4 quants from Unsloth. Thank you for your work on this! It's highly appreciated. |
@sbrnaderi Use official template. See comment |
@ServeurpersoCom It’s already ready for GLM-4.5, GLM-4.6, and Minimax-M2. Only a few minor issues remain for potential future cases. |
@thomasjfox The issue you quoted isn’t related to your crash. Tt never triggers in my tests with GLM or Minimax. And GLM and Minimax never generate something like that even with very long context length. Could you please provide more logs so I can investigate why the crash occurred? It’ll help me identify any potential underlying bugs more quickly. |
…ranch16 — unified XML tool-call parser + streaming reasoning + GLM4.5/4.6 + MiniMax-M2
MiniMax-M2-230B-A10B: GLM-4.5-Air-106B: |
I'll try to help and nail down the issue. The python script is from $dayjob, so I can't share the full log. This is the last line before the crash using the original script and unchanged code from the PR: I tried to reproduce with a random, open source python script, but that didn't work. Two things caught my eye in the full log:
When I grep the whole log with the generated output from MiniMax M2, I don't see any "minimax:tool_call" output in there at all. I will add debug output to the code and play around some more. |
|
The regex crash is because of the reverse regex generated used to find a partial match. The regex contains To avoid it, you can search for a literal such as the start tag, then proceed with regex. That will move the internal position in the builder and limit the search window for regex matching. |
There was a problem hiding this comment.
I'm not a fan of the generalizations done here. I understand there is a desire to converge the parsing of XML-based tool calling, but I believe this approach is problematic. To sum it up, the interface does not feel ergonomic and certain parts are very hacky.
Regardless, I left some comments to help improve it (in my opinion).
Other notes:
-
Tool calls should not result in any content after them. The model will emit a stop token, but also the grammar should constrain it from producing anything else.
-
I don't believe code should be left "in case we need it." It can be added when needed. Many times, YAGNI.
| // make a GBNF that accept any strings except those containing any of the forbidden strings. | ||
| std::string make_gbnf_excluding(std::vector<std::string> forbids) { | ||
| constexpr auto charclass_escape = [](unsigned char c) -> std::string { | ||
| if (c == '\\' || c == ']' || c == '^' || c == '-') { | ||
| std::string s = "\\"; | ||
| s.push_back((char)c); | ||
| return s; | ||
| } | ||
| if (isprint(c)) { | ||
| return std::string(1, (char)c); | ||
| } | ||
| char buf[16]; | ||
| snprintf(buf, 15, "\\x%02X", c); | ||
| return std::string(buf); | ||
| }; | ||
| constexpr auto build_expr = [charclass_escape](auto self, const std::vector<std::string>& forbids, int l, int r, int depth) -> std::string { | ||
| std::vector<std::pair<unsigned char, std::pair<int,int>>> children; | ||
| int i = l; | ||
| while (i < r) { | ||
| const std::string &s = forbids[i]; | ||
| if ((int)s.size() == depth) { | ||
| ++i; | ||
| continue; | ||
| } | ||
| unsigned char c = (unsigned char)s[depth]; | ||
| int j = i; | ||
| while (j < r && (int)forbids[j].size() > depth && | ||
| (unsigned char)forbids[j][depth] == c) { | ||
| ++j; | ||
| } | ||
| children.push_back({c, {i,j}}); | ||
| i = j; | ||
| } | ||
| std::vector<std::string> alts; | ||
| if (!children.empty()) { | ||
| std::string cls; | ||
| for (auto &ch : children) cls += charclass_escape(ch.first); | ||
| alts.push_back(std::string("[^") + cls + "]"); | ||
| } | ||
| for (auto &ch : children) { | ||
| std::string childExpr = self(self, forbids, ch.second.first, ch.second.second, depth+1); | ||
| if (!childExpr.empty()) { | ||
| std::string quoted_ch = "\""; | ||
| if (ch.first == '\\') quoted_ch += "\\\\"; | ||
| else if (ch.first == '"') quoted_ch += "\\\""; | ||
| else if (isprint(ch.first)) quoted_ch.push_back(ch.first); | ||
| else { | ||
| char buf[16]; | ||
| snprintf(buf, 15, "\\x%02X", ch.first); | ||
| quoted_ch += buf; | ||
| } | ||
| quoted_ch += "\""; | ||
| std::string branch = quoted_ch + std::string(" ") + childExpr; | ||
| alts.push_back(branch); | ||
| } | ||
| } | ||
| if (alts.empty()) return ""; | ||
| std::ostringstream oss; | ||
| oss << "( "; | ||
| for (size_t k = 0; k < alts.size(); ++k) { | ||
| if (k) oss << " | "; | ||
| oss << alts[k]; | ||
| } | ||
| oss << " )"; | ||
| return oss.str(); | ||
| }; | ||
| if (forbids.empty()) return "( . )*"; | ||
| sort(forbids.begin(), forbids.end()); | ||
| std::string expr = build_expr(build_expr, forbids, 0, forbids.size(), 0); | ||
| if (expr.empty()) { | ||
| std::string cls; | ||
| for (auto &s : forbids) if (!s.empty()) cls += charclass_escape((unsigned char)s[0]); | ||
| expr = std::string("( [^") + cls + "] )"; | ||
| } | ||
| if (forbids.size() == 1) | ||
| return expr + "*"; | ||
| else | ||
| return std::string("( ") + expr + " )*"; | ||
| } |
There was a problem hiding this comment.
This is only used once for a single literal. It seems crazy to try to handle every edge case. It's a common pattern in seeing throughout this PR.
Keep it simple, only produce an expression to exclude a single literal. E.g ( [^a] | "a" [^b] | ... )*
There was a problem hiding this comment.
Could you provide an example that the current implementation fails to handle? I believe all cases should already be covered.
| } | ||
| GGML_ASSERT(!key_val_sep.empty()); | ||
|
|
||
| constexpr auto encode_to_safe = [](const std::string &in) { |
There was a problem hiding this comment.
The grammar builder already cleans the rule name with add_rule(), no need to do it yourself. This is also over engineered for what it does. Remove and just build the rule name directly.
There was a problem hiding this comment.
I don't know about that. I'll delete it later.
| for (auto &s : data.preserved_tokens) { | ||
| s.resize(std::distance(s.begin(), std::find_if(s.rbegin(), s.rend(), [](unsigned char ch) { | ||
| return !std::isspace(ch); | ||
| }).base())); | ||
| size_t start = 0; | ||
| while (start < s.size() && std::isspace(static_cast<unsigned char>(s[start]))) { | ||
| ++start; | ||
| } | ||
| if (start != 0) { | ||
| s.erase(0, start); | ||
| } | ||
| } | ||
| data.preserved_tokens.erase(std::remove_if( | ||
| data.preserved_tokens.begin(), | ||
| data.preserved_tokens.end(), | ||
| [](const std::string &s) { return s.size() < 2; } | ||
| ), data.preserved_tokens.end()); | ||
| sort_uniq(data.preserved_tokens); |
There was a problem hiding this comment.
Here is where we start to see problems with this "generalized" approach. Models may have special tokens that include whitespace. By stripping it, you will not include the proper token.
There was a problem hiding this comment.
It doesn’t break anything for now. As you mentioned:
- I don't believe code should be left "in case we need it." It can be added when needed. Many times, YAGNI.
Removing the whitespace actually makes the implementation simpler.
|
|
||
| std::string param_rules; | ||
| if (parameters.contains("properties")) { | ||
| std::vector<std::string> requiredParameters; |
There was a problem hiding this comment.
Why not an std::unordered_set?
There was a problem hiding this comment.
Hmm… I’m not sure I fully understand your point. Could you explain a bit more why we should use a more complex std::unordered_set instead of a simple std::vector?
| // grammar trigger for tool call | ||
| data.grammar_lazy = true; | ||
| data.grammar_triggers.push_back({ COMMON_GRAMMAR_TRIGGER_TYPE_WORD, form.scope_start + form.tool_start }); |
There was a problem hiding this comment.
What if tool_choice = required? As it stands, that seems like it would be a nightmare to implement in a generalized fashion.
There was a problem hiding this comment.
This part of the code is copied from the original chat.cpp, as many other models use it. It’s a temporary but simple solution for now.
Maybe I will implement a full grammar for handling thinking blocks, markdown content, and tool calls later. : )
| * Parse content uses reasoning and XML-Style tool call | ||
| * TODO: Note that form.allow_toolcall_in_think is not tested yet. If anyone confirms it works, this comment can be removed. | ||
| */ | ||
| inline void parse_msg_with_xml_tool_calls(common_chat_msg_parser & builder, const struct xml_tool_call_format & form, const std::string & start_think = "<think>", const std::string & end_think = "</think>") { |
There was a problem hiding this comment.
Seems like a very large function for inline.
Nonetheless, I don't think this is necessary. There is already a reasoning parsing function and it should be preferred.
There was a problem hiding this comment.
This function is only used once, and cannot be used outside of this file. Inlining it shouldn’t be a problem, I think.
| } | ||
| }; | ||
| // Escape string literal to regex that match the literal | ||
| constexpr auto escape_regex = [](const std::string &s) { |
There was a problem hiding this comment.
What's wrong with the existing regex_escape() function? Also seems way over engineered...
There was a problem hiding this comment.
I reviewed it earlier and found it unreliable for my use case. It would be a nightmare to debug my parser using that. So I implemented a simpler and more predictable version for the parser.
| // handle unclosed top-level primitive | ||
| if (err_loc.position != 0 && !healing_marker.empty() && err_loc.stack.empty()) { | ||
| std::string str(it, temptative_end); | ||
| const auto & magic_seed = out.healing_marker.marker = healing_marker; | ||
| if (can_parse(str + "\"")) { | ||
| // Was inside an string | ||
| str += (out.healing_marker.json_dump_marker = magic_seed) + "\""; | ||
| } else if (str[str.length() - 1] == '\\' && can_parse(str + "\\\"")) { | ||
| // Was inside an string after an escape | ||
| str += (out.healing_marker.json_dump_marker = "\\" + magic_seed) + "\""; | ||
| } else { | ||
| // TODO: handle more unclosed top-level primitive if the stack was empty but we got an error (e.g. "tru", "\"", etc...) | ||
| // fprintf(stderr, "Closing: TODO\n"); | ||
| return false; | ||
| } | ||
| out.json = json::parse(str); | ||
| it = temptative_end; | ||
| return true; | ||
| } |
…ranch16 — unified XML tool-call parser + streaming reasoning + GLM4.5/4.6 + MiniMax-M2
10 participants
Generalized and streaming-capable XML-style tool-call parsing with grammar enforcement and automatic template fixing.
Based on PR #15904, this patch introduces a generalized implementation for almost all XML-style tool-call formats.
Grammar-constrained tool-call outputs
Tool-call messages generated by the model are now strictly validated against a defined grammar.
A new automatic grammar generator simplifies the process of creating grammars for new models.
This ensures that all tool-call outputs are well-formed, structurally consistent, and reliably parsed.
Streaming support for tool-call parsing
The parser now supports streaming parsing, enabling incremental processing of tool-call messages as they are generated.
This enhancement improves responsiveness and allows real-time interaction during model inference.
Automatic chat-template fixing
A lightweight Jinja2-based patcher has been added to automatically fix official chat templates before use.
With this change, official templates now work out of the box, eliminating the need for custom modifications.
In-context reasoning
The parser now supports multiple reasoning blocks within a single generation, even when interleaved with tool calls.
All reasoning content is preserved. No information is lost during parsing or streaming.
Additional Notes
--reasoning-format none-lv 1in the command line to enable more detailed logging.